-
-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend install prompt functionality #2669
Extend install prompt functionality #2669
Conversation
There is |
I think this is too complicated.
|
I must say I thought the same thing too. Especially the temp env thing. |
Yeah over complication is a fair point. I definitely worried that as I put this together. This might be a step too far into making a new duplicative repl mode |
I find myself doing this a lot Maybe we should fix being able to activate test dependencies (without having to drop support for 1.0 instead). |
I personally don't care about the temp environment bit (that was @oxinabox's request), but I really want the "other" option. It's really common to be working on a project and then do, say,
The temp thing could potentially be addressed folded into the julia> using JSON3
│ Package JSON3 not found, but a package named JSON3 is available from a
│ registry.
│ Install package?
│ (@v1.8) pkg> add JSON3
└ (y/n/o) [y]: o
julia> using JSON3
│ Package JSON3 not found, but a package named JSON3 is available from a registry.
│ Install package?
│ (Foo) pkg> add JSON3
└ Select environment:
> 1: `~/dev/Foo` (@)
2: `~/.julia/environments/v1.8` (@#.#)
t: activate and install into new temporary environment That way the first menu hardly changes and people who don't care about this aren't overwhelmed with options. |
Ah, see, I just put these in my global shared environment. After a while I have the test and dev dependencies of everything I'm working on in there and it stops being a problem. |
Updated to the last suggestion, although I couldn't work out how to make the prefix keybindings work in TerminalMenus |
Updated with working keybindings for the
For instance, pressing |
I'm getting this on Julia master: ERROR: MethodError: no method matching REPL.TerminalMenus.Config(; keybindings=['1', '2', 't'])
Closest candidates are:
REPL.TerminalMenus.Config(; charset, cursor, up_arrow, down_arrow, updown_arrow, scroll_wrap, ctrl_c_interrupt) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:45 got unsupported keyword argument "keybindings"
REPL.TerminalMenus.Config(::Char, ::Char, ::Char, ::Char, ::Bool, ::Bool) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:6 got unsupported keyword argument "keybindings"
REPL.TerminalMenus.Config(::Any, ::Any, ::Any, ::Any, ::Any, ::Any) at /Users/stefan/dev/julia/usr/share/julia/stdlib/v1.8/REPL/src/TerminalMenus/config.jl:6 got unsupported keyword argument "keybindings" |
|
Ah, ok! Will try again on Monday but I'm fine with this being merged before then. |
@fredrikekre @KristofferC what do you think about this simpler form? I quite like it. I think there's also a nice side-effect of highlighting the env stacking in a way that's not visible elsewhere |
Can you describe your workflow? Is it something like this:
and with this PR it will reduce to
Doesn't seem like much of an improvement to me.
I guess I tend to just install I am not super opposed to the select env to install option, but I think the temp thing does too much and messes with the users active project etc. |
This seems pretty annoying — I'm already annoyed that I need to edit the project file to modify the UUID of stdlibs, at least with normal packages I'm developing I don't have to do that. Now I have to keep remembering not to accidentally commit extra dev dependencies that I added to the project just to test it? I mean, I guess I could live with it, but I find my current approach of just adding all the dev & test tools to my shared global environment much preferable. Compatibility with the environment of the project being developed has never been an actual issue for me doing this. I know that in theory it could be if there was some common dependency that was incompatible, but in practice it just hasn't been. Btw, having subprojects would address the "resolved together" issue and if we added subprojects to the load path then it would address that issue as well since we'd resolve the subprojects at the same as we resolve the main project and then load the tools from the subproject environment. I think it would be nice to have a way to have the dev and test subprojects of the current active environment in the load path after the main project. In that situation, the "other" option here could also give the option to add packages to subprojects (possibly whether they're in the load path or not). |
Btw, just to compare, my preferred workflow with this PR would become this:
So I'd say that's by far the shortest, least annoying workflow described here. And again: if you don't like this workflow, it only adds a |
I do think that part of your point is that creating a temporary environment and activating it is a bit weird — you have to switch back to your previous active environment to continue your work anyway. My option doesn't have that issue because it doesn't activate a different environment, it just installs into a different one, leaving the active environment the same. It seems like what one really wants for @oxinabox's workflow is to create a temporary environment, add the test dependency to it and then add it to the load path right after the active environment. But that's a lot to bake into a convenience thing like this. That's why I think it might be better for that workflow to combine something like |
It'd be good to figure out what to include in this PR. |
So I think the behavour I actually want is not: Which I agree is a pretty complex operation. re: @fredrikekre
yeah sure. What I want to be able to do is copy and paste in the block of code that is at the top of my sing Base.Broadcast: broadcastable
using ChainRules
using ChainRulesCore
using ChainRulesTestUtils
using ChainRulesTestUtils: rand_tangent, _fdm
using Compat: hasproperty, only, cispi, eachcol
using FiniteDifferences
using FiniteDifferences: rand_tangent
using LinearAlgebra
using LinearAlgebra.BLAS
using LinearAlgebra: dot
using Random
using StaticArrays
using Statistics
using Test
ChainRulesTestUtils.enable_tangent_transform!(Thunk)
Random.seed!(1) # Set seed that all testsets should reset to. Future
Total 4 steps Realistic Current
Total 9-12 steps Ideal Current if I never made mistakes
Total 6 steps The thing is not to compare the future again the version where I never make mistakes. |
On slack, @StefanKarpinski made the nice suggestion of adding So the |
Marking this for 1.7 backport. Hopefully that's ok as it's a tweak to an already added 1.7 new feature |
This is an additional feature so I don't think it should be backported. It's fine for 1.7 to include the |
Adds two options and a help mode, while intending to not take up more room or overload with information.
@StefanKarpinski @oxinabox would you mind giving this a go? The experience is a little hard to capture fully in screenshots.
Also, is there a better way to show the
LOAD_PATH
and a function to do the conversion?i.e. instead of
["@", "@v#.#"]
show["/path/to/MyProject", "@v1.8"]
Note that I dropped
@stdlib
from the options